-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
querier: Adds additional stats field in query api #4176
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this issue.
I think this can be enabled by default and we don't need a flag.
Also please add some unit tests.
Alright, ill update the changes |
@yeya24 PTAL 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small nits. Also it would be good if you can update the changelog
Also a stupid question, when and when not to update changelog ? |
Hi @someshkoli, please rebase on main then we can get CI fixed. |
Signed-off-by: someshkoli <[email protected]>
Signed-off-by: someshkoli <[email protected]>
Signed-off-by: someshkoli <[email protected]>
Signed-off-by: someshkoli <[email protected]>
Signed-off-by: someshkoli <[email protected]>
Signed-off-by: someshkoli <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Will merge on green.
yey all greens 🎉 |
When your pr changes some known behaviors or are user-facing. IMO this change provides a new param so it is user-facing. |
Signed-off-by: someshkoli [email protected]
Changes
Adds option to pass additional paramater
stats
in form data for query API.Closes #4170
Verification
need to add some endpoint tests